fix: reimplement password reset #20966
Conversation
|
Size Change: 0 B Total Size: 821 kB ℹ️ View Unchanged
|
…/posthog into fix/reimplement-pw-reset
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
There was a problem hiding this comment.
Generally I approve of this 👍
Just needs to incorporate the fixes from #20993
posthog/api/user.py
Outdated
| @@ -67,6 +76,10 @@ class UserEmailVerificationThrottle(UserRateThrottle): | |||
| rate = "6/day" | |||
|
|
|||
|
|
|||
| class UserPasswordResetThrottle(UserRateThrottle): | |||
| rate = "6/day" | |||
There was a problem hiding this comment.
I think this is the source of all our issues. This has no "scope" which means all of the throttlers that derive from the base UserRateThrottle will be affecting each others rate limiting.
This attempts to fix that problem #20993
| logger = structlog.get_logger(__name__) | ||
|
|
||
|
|
||
| class PHPasswordResetTokenGenerator(PasswordResetTokenGenerator): |
There was a problem hiding this comment.
Personally I think this should be in a sub folder like the services folder to differentiate it from api routers
| @@ -413,6 +426,139 @@ def request_email_verification(self, request, **kwargs): | |||
|
|
|||
| return Response({"success": True}) | |||
|
|
|||
| # Password reset actions | |||
|
|
|||
| def validate_password_reset_token(self, user_uuid: str, token: str) -> bool: | |||
There was a problem hiding this comment.
Would be good to indicate this as private
| def validate_password_reset_token(self, user_uuid: str, token: str) -> bool: | |
| def _validate_password_reset_token(self, user_uuid: str, token: str) -> bool: |
| methods=["POST"], | ||
| detail=True, | ||
| permission_classes=[AllowAny], | ||
| throttle_classes=[UserPasswordResetThrottle], |
There was a problem hiding this comment.
Each of these throttle classes arguably should have their own throttle scope
For example what we don't want is that
- Request to reset password succeeds but is the second-to-last before being throttled
- Email is sent so they click the link
- The attempt to verify the password and it succeeds but is the last before being throttled
- Now they actually attempt the reset - rate limited!
|
Ended up not being needed / was different problem! |
Problem
A few days ago password resets stopped working. Users are sent three reset emails with different tokens, all of which when clicked say the token is invalid or expired.
I added error logging and oddly the errors are never triggered. It's almost like our password reset implementation never gets hit - it's just using the default one.
The email verification flow works in a very similar way to password reset, but instead of being its own endpoint it's an action on the user. This email verification flow still works just fine. As a relatively quick fix I'm going to reimplement the password reset flow in the same way that email verification works to see if that fixes the problem.
Changes
Reimplements password reset flow as actions on the user instead of an independent endpoint.
To do:
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Should be fine
How did you test this code?